-
Notifications
You must be signed in to change notification settings - Fork 21
test function on_xtype #192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #192 +/- ##
==========================================
- Coverage 96.53% 96.27% -0.27%
==========================================
Files 8 8
Lines 289 322 +33
==========================================
+ Hits 279 310 +31
- Misses 10 12 +2
|
I am a little unclear what this is doing. Could you maybe hit down some words to explain the high level view? |
I think this is for checking if |
Is this a new function? One that can be used instead of |
It's an old function. This PR is to check that it returns the right array and raise an error if it's an invalid xtype. It doesn't replace on_tth but simply calls on_tth if xtype is tth. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls see inline comment
tests/test_diffraction_objects.py
Outdated
@pytest.mark.parametrize("inputs, expected", params_on_xtype) | ||
def test_on_xtype(inputs, expected): | ||
test = DiffractionObject() | ||
test.on_tth = np.array([inputs[1], inputs[0]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not really how DO's should be used. The tth, q and d arrays are somehow all different versions of the same array so we shouldn't be able to set them independently like this. If we can now this is not desirable behavior, so we probably don't want a test that does this. We should probably create a set
property operator that generates all the arrays whenever a single one is specified in this way. But for sure, we don't want a test with this undesirable behavior....
@sbillinge ready for review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please see inline.. We can discuss, but this should behave the same as DO.on_q()
etc., and I think they should maybe return list of 1D arrays.
@@ -376,25 +376,26 @@ def scale_to(self, target_diff_object, xtype=None, xvalue=None): | |||
return scaled | |||
|
|||
def on_xtype(self, xtype): | |||
""" | |||
f""" | |||
return a 2D np array with x in the first column and y in the second for x of type type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to return a list of two 1D arrays
|
||
Returns | ||
------- | ||
|
||
a 2D np array with x and y data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one small tweak inline
tests/test_diffraction_objects.py
Outdated
with pytest.raises( | ||
ValueError, | ||
match=re.escape( | ||
f"WARNING: I don't know how to handle the xtype, 'invalid'. Please rerun specifying an " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think are now raising an exception, right? So this isn't a warning. Just remove WARNING:
Python will handle the rest.
issue #186: test function for
on_xtype